Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: add bleeding check #88

Merged
merged 2 commits into from
Jan 3, 2025
Merged

ci: add bleeding check #88

merged 2 commits into from
Jan 3, 2025

Conversation

Young-Flash
Copy link
Collaborator

No description provided.

Copy link

peter-jerry-ye-code-review bot commented Jan 3, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Here are three observations from the git diff output:

  1. Inconsistent Naming Convention in C Code:

    • In the file fs/internal/ffi/native_stub.c, the function moonbit_array_length is changed to Moonbit_array_length. This introduces inconsistency in naming conventions. Typically, C functions use lowercase or snake_case for consistency and readability. The change to Moonbit_array_length (with an uppercase M) might be unintentional or could lead to confusion. Ensure that the naming convention is consistent throughout the codebase.
  2. Potential Issue with Windows Installation Path:

    • In the bleeding-build job, the Windows installation path is hardcoded as C:\Users\runneradmin\.moon\bin. This assumes that the GitHub Actions runner always uses the runneradmin user, which might not always be the case. This could cause issues if the runner uses a different user. Consider using an environment variable or a more dynamic approach to determine the correct path.
  3. Redundant Test Commands:

    • In the bleeding-build job, the moon test step runs the same tests twice (once with --release and once without). For example:
      moon test --target all --serial --release
      moon test --target all --serial
      This redundancy might not be necessary and could increase the execution time of the workflow. Consider whether both commands are needed or if one can be removed to optimize the workflow.

These are the key observations from the provided git diff output. Let me know if you need further clarification or assistance!

@Young-Flash Young-Flash merged commit 6a7bad7 into main Jan 3, 2025
8 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant